preserve Identity Center imported group member label #48785
Conversation
lib/services/local/access_list.go
Outdated
| if existingMember.Origin() == common.OriginAWSIdentityCenter { | ||
| newMember.Metadata.Labels = existingMember.GetAllLabels() | ||
| } |
There was a problem hiding this comment.
It might be helpful to add a comment here explaining the purpose of this code.
As I understand it, the frontend currently doesn’t support modifying user member labels. This code appears to preserve the existing label state from the previous object, as the UpsertAccessListWithMembers handling in the frontend leaves member labels empty.
Would it be possible to use newMember.AddedBy.AddedBy = "IGS Service" instead of labels? If so, it might allow us to avoid making this change altogether.
My motivation is that this logic of label preservation/forwarding is quite complex and technical dept so if possible i would suggest to avoid adding additional label forwarding logic.
If we proceed with this change the UpdateAccessListMember endpoint should be also aligned:
https://github.com/gravitational/teleport.e/blob/9017e6e8ab8ddea071a644cde785f1e7f637b6f6/lib/web/accesslist.go#L237
There was a problem hiding this comment.
Curious, where is UpsertAccessListMember and UpdateAccessListMember used? I stumbled upon that yesterday but couldn't find its usage so left it untouched.
The UpsertAccessListMember method is invoked from h.POST("/enterprise/accesslist/:accessListId/members", h.WithAuth(p.addMembersToAccessList)) API but I could not find it called from the Web UI.
There was a problem hiding this comment.
Probably this addMembersToAccessList endpoint was likely added and exposed to the frontend in initial implementation of access list , and later replaced by UpsertAccessListWithMembers.
There was a problem hiding this comment.
Yeah looks like that. Though still added it in case these functions gets picked up in future d7e6ba0
flyinghermit
requested review from
r0mant and
tcsc
and removed request for
Joerger and
fspmarshall
flyinghermit
added
no-changelog
lib/services/local/access_list.go
Outdated
| // conditionallyPreserveAWSIdentityCenterLabels preserves member labels if | ||
| // it originated from AWS Identity Center plugin. | ||
| // The Web UI does not preserve metadata labels so this function should be called | ||
| // in every update/upsert member calls. |
There was a problem hiding this comment.
The comment should explain why we need to preserve them. Tbh it's still not super clear to me. What scenario is this needed for?
There was a problem hiding this comment.
The Web UI does not preserve metadata labels so this function should be called
in every update/upsert member calls.
That is meant to explain the why but I can see it failed to convey the message.
See this issue I created yesterday https://github.com/gravitational/teleport.e/issues/5415. Right now when members are added/removed from the access list using the web UI, we do not preserve existing labels and metadata name. Its a full replacement of old members with new member struct. That calls for preserving existing labels otherwise it will be updated to be nil since labels are not handled at all https://github.com/gravitational/teleport.e/blob/master/lib/web/accesslist.go#L250C1-L269C2
There was a problem hiding this comment.
Any suggestion on the commentary? I am falling short on how to explain that otherwise. I put ticket link there for reference.
| // it originated from AWS Identity Center plugin. | ||
| // The Web UI does not currently preserve metadata labels so this function should be called | ||
| // in every update/upsert member calls. | ||
| // Remove this function once https://github.com/gravitational/teleport.e/issues/5415 is addressed. |
There was a problem hiding this comment.
Hmm, wouldn't it make more sense to update the memberToAccessListMember function that you linked in that ticket to keep the labels when creating a new member then?
There was a problem hiding this comment.
I did look at it before creating the ticket and its not going to be a trivial change. I don't even know if its a feature or a bug cause the way it is currently implemented, its a single API where we upsert entire access list, even if its just a single member addition/removal https://github.com/gravitational/teleport.e/blob/master/lib/web/plugin.go#L285.
Preserving older state is not something exclusively done for the identity center. See prior work on https://github.com/gravitational/teleport/blob/master/lib/services/local/access_list.go#L729C1-L735C58. Looks like we settled on with the current implementation? Anyway, I think updating that API to handle label and Idmetadata name is best tackled separately.
There was a problem hiding this comment.
I understand we replace the list, but from the ticket description the impression I got was that we just do not add member.Metadata.Labels in that function. Is that not the case?
I don't have objection to keep this implementation for now but trying to understand why passing the labels to the newly created member in that function does not work.
There was a problem hiding this comment.
Sorry for not being clear. Another attempt below:
that we just do not add member.Metadata.Labels in that function. Is that not the case?
Yup correct. But that is also due to the fact that the memberToAccessListMember function does not have access to the metadata label because we do not send them to the UI in GET Access List https://github.com/gravitational/teleport.e/blob/master/lib/web/ui/accesslist.go#L10C1-L25C2. And as a result the UI does not handle metadata values, nor it send them back in the upsert request. https://github.com/gravitational/teleport.e/blob/master/lib/web/ui/accesslist.go#L38C1-L42C2. Hence looses any labels applied to the member.
So besides the memberToAccessListMember function, Access List GET and PUT apis along with Web UI needs to be updated to solve that issue.
public-teleport-github-review-bot
bot
removed the request for review
from tcsc
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@flyinghermit See the table below for backport results.
|
Implemented for https://github.com/gravitational/teleport.e/pull/5422